Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

gh-87063: Add closed attribute to multiprocessing.Process #125838

Open
wants to merge 20 commits into
base: main
Choose a base branch
from
Open

gh-87063: Add closed attribute to multiprocessing.Process #125838

wants to merge 20 commits into from

Conversation

rruuaanng
Copy link
Contributor

@rruuaanng rruuaanng commented Oct 22, 2024 •
edited
Loading

for example:

import multiprocessing


def f():
    print('hello')
    print('world')

if __name__ == "__main__":
    p = multiprocessing.Process(target=f)
    p.start()
    p.join()

    # In a more context, it's closed
    p.close()
    
    # Currnet context
    # It's thrown a ValueError
    if p.exitcode == 0:
        pass

    # Changed
    if not p.is_closed():
        print(p.exitcode)

@bedevere-app bedevere-app bot mentioned this pull request Oct 22, 2024
Open
@rruuaanng
Copy link
Contributor Author

It seems that this NEWS error can be ignored.

picnixz
picnixz reviewed Oct 22, 2024
View reviewed changes
Copy link
Contributor

@picnixz picnixz left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

More generally, required CI checks must never be ignored except for NEWS missing / issue missing since normal users cannot apply the skip news/issue labels themselves.

Misc/NEWS.d/next/Library/2024-10-22-19-52-15.gh-issue-87063.fR-wJa.rst Outdated Show resolved Hide resolved
Lib/test/_test_multiprocessing.py Outdated Show resolved Hide resolved
Lib/multiprocessing/process.py Show resolved Hide resolved
picnixz
picnixz reviewed Oct 24, 2024
View reviewed changes
Doc/library/multiprocessing.rst Outdated Show resolved Hide resolved
Doc/library/multiprocessing.rst Outdated Show resolved Hide resolved
Lib/test/_test_multiprocessing.py Outdated Show resolved Hide resolved
Misc/NEWS.d/next/Library/2024-10-22-19-52-15.gh-issue-87063.fR-wJa.rst Outdated Show resolved Hide resolved
@erlend-aasland erlend-aasland changed the title gh-87063: Add is_closed method to multiprocessing.Process gh-87063: Add is_closed attribute to multiprocessing.Process Oct 25, 2024
@erlend-aasland erlend-aasland changed the title gh-87063: Add is_closed attribute to multiprocessing.Process gh-87063: Add closed attribute to multiprocessing.Process Oct 25, 2024
erlend-aasland
erlend-aasland requested changes Oct 26, 2024
View reviewed changes
Copy link
Contributor

@erlend-aasland erlend-aasland left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

PR author did not listen to feedback. Requesting changes to make this point clearer.

@bedevere-app
Copy link

bedevere-app bot commented Oct 26, 2024

A Python core developer has requested some changes be made to your pull request before we can consider merging it. If you could please address their requests along with any other requests in other reviews from core developers that would be appreciated.

Once you have made the requested changes, please leave a comment on this pull request containing the phrase I have made the requested changes; please review again. I will then notify any core developers who have left a review that you're ready for them to take another look at this pull request.

@rruuaanng
Copy link
Contributor Author

Sorry for my misunderstanding. I thought it was to change NEWS to what's NEWS, but it was actually to add to it. I'm sorry for that. I've made change :)

picnixz
picnixz reviewed Oct 26, 2024
View reviewed changes
Misc/NEWS.d/next/Library/2024-10-22-19-52-15.gh-issue-87063.fR-wJa.rst Outdated Show resolved Hide resolved
Doc/whatsnew/3.14.rst Outdated Show resolved Hide resolved
Doc/whatsnew/3.14.rst Outdated Show resolved Hide resolved
picnixz
picnixz reviewed Oct 26, 2024
View reviewed changes
Lib/test/_test_multiprocessing.py Outdated Show resolved Hide resolved
Lib/test/_test_multiprocessing.py Show resolved Hide resolved
@rruuaanng
Copy link
Contributor Author

Thanks to Bénédikt for taking responsibility for this PR. And, thanks to erlend for pointing out my mistake.👍

picnixz
picnixz reviewed Oct 26, 2024
View reviewed changes
Copy link
Contributor

@picnixz picnixz left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

A final nitpick. Concerning the naming of the attribute, I suggested a property + closed but if @gpshead feels that it should be a method + is_closed(), then I'm sorry in advance :) (just recalling that my rationale was that multiprocessing.connection.Connection uses the closed property but I don't know if properties are preferred over true methods when we are just returning a private attribute).

Doc/whatsnew/3.14.rst Show resolved Hide resolved
@rruuaanng
Copy link
Contributor Author

I've made change. Thank for your review!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@picnixz picnixz picnixz left review comments

@gpshead gpshead Awaiting requested review from gpshead gpshead is a code owner

@erlend-aasland erlend-aasland Awaiting requested review from erlend-aasland

Assignees
No one assigned
Labels
awaiting changes
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@rruuaanng @picnixz @erlend-aasland